[featured] fix stale FEATURE entry after concurrent package uninstall#384
[featured] fix stale FEATURE entry after concurrent package uninstall#384DavidZagury wants to merge 6 commits into
Conversation
When sonic-package-manager uninstalls a package it deletes the FEATURE
entry from CONFIG_DB. Because featured unblocks spm (via STATE_DB) before
finishing its own handler, the entry can be gone by the time featured tries
to write back to it. Two code paths independently recreate the deleted
entry, leaving a ghost record that survives the uninstall and appears in
"show feature status":
1. sync_feature_scope(): called after a successful disable_feature(), it
calls mod_entry({has_per_asic_scope, has_global_scope}) on the already-
deleted key. Redis HSET recreates the hash with only those two fields
(no 'state'), so the next notification arrives with state=None and
featured logs "ERR: Unexpected state value 'None'".
There is also a TOCTOU gap: spm can delete the entry between the
mod_entry calls and the post-write read, producing the same partial
entry.
2. resync_feature_state(): called when update_feature_state() fails (e.g.
systemctl start fails because the service file was already removed by
the concurrent uninstall). get_entry() returns {} for a deleted key,
_feature_state_is_template(None) evaluates True, and mod_entry writes
{state: disabled} back - recreating the entry with no other fields.
The same function also completes partial entries left by (1): a non-
empty entry with no 'state' key passes the deleted-entry guard and
receives a state field, producing a 3-field ghost record.
Fix all paths:
- sync_feature_scope: after the two mod_entry writes, re-read the entry.
If it exists but has no 'state' key, the write raced with deregister;
delete the partial entry immediately.
- resync_feature_state: return early if the entry is fully absent. If
the entry exists but has no 'state' key, delete it rather than
completing the ghost record.
- update_feature_state: suppress the ERR log when state is None, since
that is a normal consequence of the race rather than a programming error.
Signed-off-by: david.zagury <davidza@nvidia.com>
test_feature_resync: the sub-case where get_entry returns None previously asserted that mod_entry would be called with the rendered state. That behaviour was the root of the bug -- resync_feature_state was recreating a FEATURE row that had been deleted by a concurrent deregister. Update the assertion to assert_not_called(), matching the new guard that returns early when the entry is absent. Add a further sub-case for a partial entry (row exists but has no 'state' key, the TOCTOU artifact left by sync_feature_scope racing with deregister): resync_feature_state must call set_entry(None) to remove the ghost record rather than completing it with mod_entry. test_sync_feature_scope_toctou: new test for the post-write TOCTOU guard in sync_feature_scope. Uses side_effect to return a live entry on the pre-write check and a partial entry (no 'state' key) on the post-write check, simulating the window where the FEATURE row is deleted and recreated by mod_entry during the race. sync_feature_scope must call set_entry(None) to clean it up. Signed-off-by: david.zagury <davidza@nvidia.com>
dd9a46e to
b10757c
Compare
… entry. Signed-off-by: david.zagury <davidza@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@saiarcot895 could you please help review? Thanks. |
There was a problem hiding this comment.
Pull request overview
Fixes a race between featured and sonic-package-manager during package uninstall that could recreate a deleted FEATURE row in CONFIG_DB with partial fields (“ghost” entry), leading to inconsistent show feature status output and spurious error logs.
Changes:
- Add post-write validation in
sync_feature_scope()to detect and immediately delete a partially recreatedFEATUREentry (missingstate). - Harden
resync_feature_state()to avoid recreating deleted entries and to delete partial entries missingstateinstead of “completing” them. - Update unit tests to cover missing/partial
FEATURErow scenarios and thesync_feature_scope()TOCTOU cleanup behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/featured |
Adds guards/cleanup to prevent recreating deleted FEATURE entries during concurrent uninstall and suppresses erroneous log noise for state=None. |
tests/featured/featured_test.py |
Extends test coverage for deleted/partial FEATURE entries and validates the new cleanup behavior. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: david.zagury <davidza@nvidia.com>
49164de to
800288d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@DavidZagury can you please take a look at copilot's comments. Also please resolve the conflicts. thanks |
@Sourabh-Kumar7 resolved |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Resolved conflict in tests/featured/featured_test.py: included both our partial-entry guard tests (test_sync_feature_scope_toctou sub-case) and the two-phase ordering tests added by sonic-net#387 (test_sync_state_field_two_phase_ordering, test_update_systemd_config_reload_parameter, test_sync_state_field_empty_table). Signed-off-by: david.zagury <davidza@nvidia.com>
d42035d to
0f6e35c
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Hi — this PR is flagged for cherry-pick to the 202605 branch (
This helps us track readiness and ensures we don't cherry-pick untested changes. Thanks! Comment by @vaibhavhd via automated tooling |
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
Why I did it
After uninstalling a SONiC application package, the feature could reappear in show feature status with an incomplete entry (state: disabled, missing auto_restart, set_owner, etc.), even though the package was fully removed. This was caused by a race between featured and sonic-package-manager during uninstall.
sonic-package-manager unblocks as soon as featured writes disabled to STATE_DB, then immediately runs deregister() which deletes the FEATURE entry from CONFIG_DB. At that point featured is still inside its handler and has not finished writing back to CONFIG_DB. Two code paths independently recreate the deleted entry with partial data:
How I did it
Three guards added to featured:
Additionally, the ERR log for state=None in update_feature_state is suppressed since state=None means the FEATURE entry was deleted by a concurrent uninstall while the notification was queued — not a programming error. Any other unexpected non-None state still logs ERR.